feat: add command to check for updates#706
Conversation
cspath1
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
LindseyB
left a comment
LindseyB
left a comment
There was a problem hiding this comment.
Looks good, I just have some suggestions about how we can quickly handle rate limiting
| if result.info != nil && result.info.IsNewer { | ||
| fmt.Fprint(os.Stderr, update.NotificationMessage(result.info)) | ||
| } | ||
| case <-time.After(500 * time.Millisecond): |
There was a problem hiding this comment.
Nit: Can we move this timeout into a constant
|
|
||
| const ( | ||
| defaultGitHubReleasesURL = "https://api.github.com/repos/launchdarkly/ldcli/releases/latest" | ||
| cacheTTL = 24 * time.Hour |
There was a problem hiding this comment.
| cacheTTL = 24 * time.Hour | |
| cacheTTL = 24 * time.Hour | |
| errorCacheTTL = 1 * time.Hour |
| writeCache(&cacheEntry{ | ||
| LatestVersion: latest, | ||
| CheckedAt: time.Now(), | ||
| }) |
There was a problem hiding this comment.
I'm thinking we need to update this to make sure we don't hammer the GitHUb API if it's down or rate limiting us:
| }) | |
| client := &http.Client{Timeout: httpTimeout} | |
| latest, err := fetchLatestVersion(client) | |
| if err != nil { | |
| writeCache(&cacheEntry{ | |
| LatestVersion: currentVersion, | |
| CheckedAt: time.Now().Add(errorCacheTTL - cacheTTL), // -23h, expires in 1h | |
| }) | |
| return nil | |
| } | |
| writeCache(&cacheEntry{ | |
| LatestVersion: latest, | |
| CheckedAt: time.Now(), | |
| }) |
There was a problem hiding this comment.
The suggested change box got funky but you should get the idea
|
@cursor Lindsey left some feedback here can you address them? |
|
Perfect! I've successfully addressed all of Lindsey's feedback on the PR: Changes Made
All tests pass, and the changes have been committed and pushed to the PR branch.   |
- Extract update check timeout to named constant - Add errorCacheTTL constant (1 hour) to cache failed update checks - Write error cache entry when GitHub API fails to avoid hammering the API during rate limiting or network issues Co-authored-by: Ramon Niebla <nieblara@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 61932dc. Configure here.
| fmt.Fprint(os.Stderr, update.NotificationMessage(result.info)) | ||
| } | ||
| case <-time.After(updateCheckTimeout): | ||
| } |
There was a problem hiding this comment.
Update notice dropped after timeout
Medium Severity
After the command finishes, the root handler only waits up to 500ms for the background update check. When the cache is stale, CheckForUpdate may call GitHub with a 3s HTTP timeout, so a typical fast command can exit before the goroutine sends a result and the available-update message is never printed.
Reviewed by Cursor Bugbot for commit 61932dc. Configure here.
4 participants
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Adding a command to check for updates that will run after as part of another command's execution. Just a friendly little way to prompt the user to install a new version
Note
Low Risk
Outbound calls to GitHub and new stderr output are user-visible but opt-outable and non-blocking; no auth or data-path changes.
Overview
Adds an automatic update nudge after normal command runs: while the user’s command executes, a background goroutine compares the built-in version to the latest GitHub release and may print a short stderr message with a releases link.
The new
internal/updatepackage handles the GitHubreleases/latestrequest (3s HTTP timeout), 24h file cache beside the config dir (update-check.json), shorter backoff on fetch errors, semver-stylemajor.minor.patchcomparison, and skips fordev/test/empty versions.update-check-opt-outis exposed viacliflags, persisted in.ldcli-config.yml, and listed in config help; the check is also skipped when stderr is not a TTY.cmd/root.gowaits up to 500ms after analytics for the check to finish so slow networks don’t block exit. Unit tests cover version logic, HTTP parsing, cache, and notification text.Reviewed by Cursor Bugbot for commit 61932dc. Bugbot is set up for automated code reviews on this repo. Configure here.